Convert warnings to exceptions in standard lib (reopened) #5004

kocsismate · 2019-12-11T15:30:43Z

Reopening #4937 after it has been rebased to current master.

nikic · 2019-12-20T11:17:46Z

ext/standard/password.c

@@ -83,7 +83,7 @@ static zend_string* php_password_make_salt(size_t length) /* {{{ */
 	zend_string *ret, *buffer;

 	if (length > (INT_MAX / 3)) {
-		php_error_docref(NULL, E_WARNING, "Length is too large to safely generate");
+		zend_value_error("Length is too large to safely generate");


This commit can be dropped, password_* changes have landed separately.

nikic · 2019-12-20T11:21:37Z

ext/standard/ftp_fopen_wrapper.c

@@ -1050,7 +1050,7 @@ static int php_stream_ftp_mkdir(php_stream_wrapper *wrapper, const char *url, in

 	if (resource->path == NULL) {
 		if (options & REPORT_ERRORS) {
-			php_error_docref(NULL, E_WARNING, "Invalid path provided in %s", url);
+			zend_value_error("Invalid path provided in %s", url);


As a general rule of thumb, I don't think we should be throwing from inside stream wrapper operations. Beyond "filename cannot contain null bytes" I think we should stick with existing warnings where file-system operations are concerned.

nikic · 2019-12-20T11:24:24Z

ext/standard/php_fopen_wrapper.c

@@ -219,7 +219,7 @@ php_stream * php_stream_url_wrap_php(php_stream_wrapper *wrapper, const char *pa

 		if ((options & STREAM_OPEN_FOR_INCLUDE) && !PG(allow_url_include) ) {
 			if (options & REPORT_ERRORS) {
-				php_error_docref(NULL, E_WARNING, "URL file-access is disabled in the server configuration");
+				zend_throw_error(NULL, "URL file-access is disabled in the server configuration");


Same here. This seems like a clear "environment configuration" case that code may need to deal with.

nikic · 2019-12-20T12:22:49Z

ext/standard/basic_functions.c

-		php_error_docref(NULL, E_WARNING, "Filename cannot be empty!");
-		RETURN_FALSE;
+		zend_value_error("Filename cannot be empty!");
+		return;


@cmb69 What do you think about moving this check into Z_PARAM_PATH? It seems to be a pretty common check (and it's also checked in php_stream_open_wrapper), but we probably don't check it everywhere we should be checking it. That would make sure it's treated consistently.

(Conversely: Are there any cases where an empty path should be accepted?)

There are at least some cases where an empty path should be accepted, e.g. for SQLite3::open() (where an empty path is supposed to create a temporary on-disk database). Generally, I think that empty paths should be forbidden right away nonetheless. Maybe change Z_PARAM_PATH to reject empty paths, and cater to the few exceptions by checking for a string and adding a manual NUL byte check? Or maybe introduce another macro (say, Z_PARAM_PATH_MAYBE_EMPTY)? The latter won't work with classic ZPP, though (don't think that would a problem though).

nikic · 2019-12-20T12:24:51Z

ext/standard/browscap.c

-			php_error_docref(NULL, E_WARNING, "browscap ini directive not set");
-			RETURN_FALSE;
+			zend_throw_error(NULL, "Browscap ini directive not set");
+			return;


Imho this is again an environment issue: Code may have to deal with the fact that browscap is not available.

nikic · 2019-12-20T12:25:22Z

ext/standard/dl.c

-		php_error_docref(NULL, E_WARNING, "Dynamically loaded extensions aren't enabled");
-		RETURN_FALSE;
+		zend_throw_error(NULL, "Dynamically loaded extensions aren't enabled");
+		return;


Similar here, environment issue.

nikic · 2019-12-20T12:25:36Z

ext/standard/dns.c

-		php_error_docref(NULL, E_WARNING, "Host cannot be empty");
-		RETURN_FALSE;
+		zend_value_error("Host cannot be empty");
+		return;


This is fine :)

nikic · 2019-12-20T12:26:13Z

ext/standard/filestat.c

@@ -354,7 +354,7 @@ static void php_do_chgrp(INTERNAL_FUNCTION_PARAMETERS, int do_lchgrp) /* {{{ */
 				option = PHP_STREAM_META_GROUP_NAME;
 				value = Z_STRVAL_P(group);
 			} else {
-				php_error_docref(NULL, E_WARNING, "parameter 2 should be string or int, %s given", zend_zval_type_name(group));
+				zend_type_error("Parameter 2 should be string or int, %s given", zend_zval_type_name(group));


This is fine, but RETURN_FALSE should be changed to return.

nikic · 2019-12-20T12:27:35Z

ext/standard/levenshtein.c

@@ -123,7 +123,7 @@ PHP_FUNCTION(levenshtein)
 	}

 	if (distance < 0 && /* TODO */ ZEND_NUM_ARGS() != 3) {
-		php_error_docref(NULL, E_WARNING, "Argument string(s) too long");
+		zend_value_error("Argument string(s) too long");


Code may want to handle this condition, as I don't think the limit is exposed (and it's a low limit -- this is not a 2G string condition).

nikic · 2019-12-20T12:28:04Z

ext/standard/proc_open.c

 					goto exit_fail;
 				}
 				if (Z_TYPE_P(ztarget) != IS_LONG) {
-					php_error_docref(NULL, E_WARNING, "Redirection target must be an integer");
+					zend_value_error("Redirection target must be an integer");


These should all be fine.

nikic · 2019-12-20T12:33:31Z

ext/standard/string.c

-		php_error_docref(NULL, E_WARNING, "Offset not contained in string");
-		RETURN_FALSE;
+		zend_value_error("Offset not contained in string");
+		return;


These are a bit borderline (I wouldn't be surprised if there is code ignoring this warning out there), but overall I think we should do this change.

nikic · 2019-12-20T12:33:57Z

ext/standard/string.c

-			php_error_docref(NULL, E_WARNING, "Offset not contained in string");
-			RETURN_FALSE;
+			zend_value_error("Offset not contained in string");
+			return;
 		}
 		p = ZSTR_VAL(haystack) + (size_t)offset;
 		e = ZSTR_VAL(haystack) + ZSTR_LEN(haystack);
 	} else {
 		if (offset < -INT_MAX || (size_t)(-offset) > ZSTR_LEN(haystack)) {


These INT_MAX should be ZEND_LONG_MAX.

kocsismate · 2019-12-20T14:35:44Z

Thanks for the thorough review! Let me sum it up:

Warnings OK to promote:
- dns_check_record(): 2f08249
- chgrp(): f289483
- proc_open(): c992c01
Warnings probably OK to promote:
- "Offset not contained in string": 761842d
Warnings might be OK to promote:
- register_shutdown_function() 30f05c1
No decision yet:
- register_tick_function(): c90cce3
- constant(): f38e877
- ini_get_all(): e6b3915 (although it also seems a configuration issue for me)

Conclusion: I shouldn't create such a big PR with many promotions next time.. :)

Can I go ahead and commit the promotions in the first category (after addressing the comment about RETURN_FALSE)?

nikic · 2019-12-20T14:39:17Z

Can I go ahead and commit the promotions in the first category (after addressing the comment about RETURN_FALSE)?

Yeah. I'd also suggest to handle Command array element %d contains a null byte for proc_open as well.

phpGH-5004

kocsismate · 2019-12-20T15:49:56Z

@nikic Ah, I made a mistake and also committed the "Offset not contained in string" one as well :/ :/ Should I revert it?

nikic · 2019-12-20T15:52:40Z

@kocsismate It's okay.

…tered GH-5004

nikic · 2020-01-03T14:58:27Z

* Warnings might be OK to promote:
  
  * `register_shutdown_function()` [30f05c1](https://github.com/php/php-src/commit/30f05c137201474bfcd5efec8e819453f6472e8f)

* No decision yet:
  
  * `register_tick_function()`: [c90cce3](https://github.com/php/php-src/commit/c90cce347cacaa33838acb9638f33407a91f082d)

For these two, I think we shouldn't do the conversion, as the exception will only get thrown on the actual call, not when the function is registered. That means it's doing to be e.g. thrown when handling shutdown functions, which is not the best place to throw exceptions (in particular, it's going to skip all remaining shutdown functions).

  * `constant()`: [f38e877](https://github.com/php/php-src/commit/f38e8779c04e04772209c6e11ca0c4289dd1aeaa)

I'm uncertain on that one... Generally, I think it would make sense to make constant() behaviorally consistent with a plain constant access. For that we should pass through the existing engine errors through, by removing the quiet flag, not changing the custom error.

  * `ini_get_all()`: [e6b3915](https://github.com/php/php-src/commit/e6b39156a1e7134c1431742c6d4b269a5af04a48) (although it also seems a configuration issue for me)

Yeah.

I think everything that should land here has landed (apart from constant maybe, which needs a different implementation), so I'll close this PR now.

kocsismate · 2020-01-03T15:07:09Z

I think everything that should land here has landed

Cool, thanks for having a look! I also think that we are ok after learning about the facts you wrote.

I'll maybe try to have a look at the constant stuff at a later time if I can do something about it.

kocsismate added 16 commits December 10, 2019 17:22

Promote warning to exception in ini_get_all() function

e6b3915

Promote warning to exception in parse_ini_file() function

80250da

Promote warnings to exceptions in dl() function

ae0b1f5

Promote warnings to exceptions in password_*() functions

0a0e75e

Promote warning to exception in get_browser() function

13d8ec6

Promote warning to exception in dns_check_record() function

2f08249

Promote warning to exception in convert_uudecode() function

e1eb864

Promote warrnings to exceptions in chgrp() function

f289483

Promote warning to exception in constant() function

f38e877

Promote warnings to exceptions in proc_open() function

c992c01

Promote warning to exception in the levenshtein() function

6a4bd43

Promote warnings to exceptions in some URL and file related functions

f53e365

Promote warnings to exceptions in string search related functions

761842d

Promote warning to exception in the convert_uudecode() function

1bb354d

Promote warnings to exceptions in register_shutdown_handler() function

30f05c1

Promote warnings to exceptions in register_tick_function() function

c90cce3

kocsismate changed the title ~~Standard exceptions (reopened)~~ Converting warnings to exceptions in the standard lib (reopened) Dec 11, 2019

kocsismate changed the title ~~Converting warnings to exceptions in the standard lib (reopened)~~ Convert warnings to exceptions in standard lib (reopened) Dec 11, 2019

nikic reviewed Dec 20, 2019

View reviewed changes

kocsismate added a commit to kocsismate/php-src that referenced this pull request Dec 20, 2019

Promote warning to exception in dns_check_record() function

bbaee1a

phpGH-5004

kocsismate added a commit to kocsismate/php-src that referenced this pull request Dec 20, 2019

Promote warrnings to exceptions in chgrp() function

f8fe49f

phpGH-5004

kocsismate added a commit to kocsismate/php-src that referenced this pull request Dec 20, 2019

Promote warnings to exceptions in proc_open() function

4cd5356

phpGH-5004

kocsismate added a commit to kocsismate/php-src that referenced this pull request Dec 20, 2019

Promote warnings to exceptions in string search related functions

5b8b567

phpGH-5004

kocsismate added a commit to kocsismate/php-src that referenced this pull request Dec 20, 2019

Promote warning to exception in dns_check_record() function

9585964

phpGH-5004

kocsismate added a commit to kocsismate/php-src that referenced this pull request Dec 20, 2019

Promote warrnings to exceptions in chgrp() function

a2d83a6

phpGH-5004

kocsismate added a commit to kocsismate/php-src that referenced this pull request Dec 20, 2019

Promote warnings to exceptions in proc_open() function

17598d5

phpGH-5004

kocsismate added a commit to kocsismate/php-src that referenced this pull request Dec 20, 2019

Promote warnings to exceptions in string search related functions

f3d5a5a

phpGH-5004

php-pulls pushed a commit that referenced this pull request Dec 20, 2019

Promote warning to exception for proc_open() when null byte is encoun…

eff56f8

…tered GH-5004

nikic closed this Jan 3, 2020

kocsismate deleted the standard-exceptions branch January 3, 2020 15:00

kocsismate added the Warning promotion label Jan 28, 2020

Convert warnings to exceptions in standard lib (reopened) #5004

Convert warnings to exceptions in standard lib (reopened) #5004

Uh oh!

Conversation

kocsismate commented Dec 11, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nikic Dec 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kocsismate commented Dec 20, 2019

Uh oh!

nikic commented Dec 20, 2019

Uh oh!

kocsismate commented Dec 20, 2019

Uh oh!

nikic commented Dec 20, 2019

Uh oh!

nikic commented Jan 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kocsismate commented Jan 3, 2020

Uh oh!

Uh oh!

nikic Dec 20, 2019 •

edited

Loading

nikic commented Jan 3, 2020 •

edited

Loading